Repository navigation
fix(compilers/openapi): report an oauth2 scheme with no flow - #727
fuad-daoud wants to merge 4 commits into
Conversation
fuad-daoud
changed the title
Resolve the conflicts with main and bring the doc comments this branch adds under the 100-word cap main now enforces. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GCzyGPq9DQ7RLfagZm2JB5
There was a problem hiding this comment.
Corrected after a second pass: I don't think this reaches the bar yet, contrary to my first summary. The verdict and the ordered list of gaps are in the PR comment below, and each point is detailed inline.
One item outside the diff: the merge commit 97b5fb2 carries an AI co-author trailer and a session URL in its body. The branch shouldn't be rewritten, so set the squash body explicitly at merge time rather than letting a COMMIT_MESSAGES paste carry it onto main.
| func oauthNoFlowDiag(c lowering.Ctx, name string, entry jsontext.Pointer) ir.Diagnostic { | ||
| return c.DiagAt(ir.SeverityWarning, diag.OAuth2NoFlow, entry, | ||
| "security scheme %q is an oauth2 scheme with no flow: "+ | ||
| "no OAuth flow is interned for it, and an OAuth2 scheme states no token endpoint without one", name) |
There was a problem hiding this comment.
The rule behind this message reports a valid, usable document. In OpenAPI 3.2.0, oauth2MetadataUrl is "URL to the OAuth2 authorization server metadata [RFC8414]", and RFC 8414 metadata states token_endpoint and authorization_endpoint. flows is REQUIRED in 3.2.0, so {type: oauth2, flows: {}, oauth2MetadataUrl: ...} is the only way to write a metadata-only scheme. Compiled under openapi: 3.2.0 it prints this warning and exits 0, and --fail-on warning then rejects a spec-valid document. #646's own premise is a scheme that "describes no way to obtain a token"; a metadata URL is one.
The PR body and diag.OAuth2NoFlow record that the URL deliberately does not exempt the entry. I disagree on the merits and would reverse it: a warning that fires on valid input that says how to get a token is a false positive, not a stricter lint. The fix is one more clause, scheme.OAuth2MetadataURL == "". On a prototype, a 3.2.0 metadata-only scheme is silent and flows: {} without the URL still reports.
The flows empty with a metadata url row would then become the silent control. It compiles under openapi: 3.1.0 today, a version that doesn't define the field, so it should compile under 3.2.0.
There was a problem hiding this comment.
Agreed, and reversed. declaresNoFlow now returns false when oauth2MetadataUrl is set, so a 3.2.0 metadata-only scheme is silent, and flows: {} without the URL still reports. The flows empty with a 3.2 metadata url row now compiles under openapi: 3.2.0 as the silent control, and the new conformance spec carries the same control (metadataOnly). Dropping the exemption reddens both. b87282c
| // OAuth2NoFlow reports an oauth2 securitySchemes entry declaring no flow: | ||
| // `flows` absent, `flows: {}`, or only keys the model does not name, which all | ||
| // lower to an empty flow list. It is reported once, at the entry's pointer, | ||
| // because the loader cannot place it: it accepts `flows: {}` and reports an |
There was a problem hiding this comment.
This sentence is more than imprecise; its premise undercuts the placement rationale. The loader's finding does carry a site: morphic validate on {type: oauth2} prints absent.yaml:7:7: ... securityScheme.flows is required for type=oauth2, a Position with no pointer, and #511 made line:col a first-class locator beside the pointer. So for the absent spelling "the loader cannot place it" is false, and the new warning is a second report of a defect the loader already reports as an error.
That leaves one defect with two diagnostics, two severities and an exit code that depends on the spelling: absent exits 1, flows: {} exits 0. That is the opposite of the PR body's "one rule for every spelling". The spellings are not alike in the spec. Absent violates a REQUIRED field, so an error is right and the loader supplies it. flows: {} is valid but unusable, so a warning is right and only the compiler can see it. The repo's precedent when a rule could be checked in both stages is compilerOwned (#502): one report, not two.
The consistent design reports only what the loader cannot see: ss.GetFlows() != nil && len(scheme.Flows) == 0. GetFlows() is nil for an absent key and non-nil for {}, so the lowering can tell them apart even though the IR cannot (the PR's stated reason for one rule). Prototype result: absent draws only the loader's error, and {} and a flows object holding only unknown keys draw the one warning. The bigger alternative is owning the whole rule in the loader (drop the library finding, report every spelling pointed). That needs a node-based predicate, since the library error has no structured field identity, and I would file it separately.
There was a problem hiding this comment.
Agreed: the loader does site the absent spelling, by line and column (absent.yaml:6:8: securityScheme.flows is required for type=oauth2), so the old sentence was wrong. The warning now fires only for what the loader cannot see, ss.GetFlows() != nil with no lowered flow. {type: oauth2} now draws only the loader's error, while {} and a flows object holding only unknown keys draw the one warning. diag.OAuth2NoFlow's record is rewritten to match. Moving the whole rule into the loader is left as the separate change you describe. b87282c
| diags = preserveUnreadFields(c, &scheme, ss, decl) | ||
| if scheme.Kind == ir.AuthKindOAuth2 && len(scheme.Flows) == 0 { | ||
| // Reported rather than refused — see oauthNoFlowDiag — and reported | ||
| // before the lines below, which is why those append into diags. |
There was a problem hiding this comment.
nit: "reported before the lines below, which is why those append into diags" describes the diff (only preserveUnreadFields changed from assignment to append) and reads as an ordering requirement the code doesn't have. "Reported rather than refused, see oauthNoFlowDiag" is the part that earns its place.
There was a problem hiding this comment.
Reworded to just "Reported rather than refused; see oauthNoFlowDiag." b87282c
| case soa.SecuritySchemeTypeOAuth2: | ||
| scheme.Kind = ir.AuthKindOAuth2 | ||
| // An absent or empty flows object lowers to nil flows, which the caller | ||
| // reports (oauthNoFlowDiag). |
There was a problem hiding this comment.
The new report reads an empty lowered flow list as "the document declares no flow", which holds only while presentFlows lists every flow field of soa.OAuthFlows. It does today: the five fields in v1.25.2 are all read, and I checked. But soa.SecurityScheme is held to its source struct by TestMechanismFieldNames_AccountForEverySourceField, and nothing holds presentFlows to soa.OAuthFlows.
If a library bump adds a flow field, a scheme declaring only that flow lowers to empty Flows. Before this PR that flow was dropped silently. Now it is dropped with a false "oauth2 scheme with no flow". A reflection test over soa.OAuthFlows beside the sibling guard would close it.
There was a problem hiding this comment.
Added TestPresentFlows_ReadsEverySourceFlowField in auth_internal_test.go, beside the sibling guard. It walks soa.OAuthFlows by reflection, sets each *soa.OAuthFlow field alone and requires presentFlows to return it. Any other field has to be listed as not-a-flow (only Extensions is). Commenting out the password candidate reddens it with "presentFlows does not read the Password flow". b87282c
| }, | ||
| }, | ||
| { | ||
| name: "no type at all", entry: `{}`, refused: true, |
There was a problem hiding this comment.
The Kind == ir.AuthKindOAuth2 half of the new condition has no row of its own: every interned row in this table is oauth2. I planted if len(scheme.Flows) == 0 { (guard dropped) at this head. Exactly one test in the auth package reddens, TestLowerSecuritySchemes_ARefdEntryRecordsFieldsWhereTheyAreWritten, and only because its exact diagnostic list gains a warning for a mutualTLS alias. Six TestConformance subtests redden too (security-schemes, security-or-and, extension-promotion, extensions-x, neutral-naming, empty-names).
So the guard is held, but incidentally, and the PR's mutation list doesn't include this one. It also shows in the list's "fire only for absent flows" bullet, which credits ARefd... with catching that mutation. With the Kind guard kept (len(scheme.Flows) == 0 replaced by ss.GetFlows() == nil), ARefd... stays green and only the three oauth2 rows redden. ARefd... reddens only when the whole condition, Kind guard included, is replaced, so what it catches there is the missing guard.
One interned non-oauth2 row (apiKey or http) asserting messagesAt(diags, diag.OAuth2NoFlow) is empty would pin the guard where the rule lives.
There was a problem hiding this comment.
Added a row of its own: an apiKey scheme carrying flows: {}. With the new GetFlows() != nil half, a plain apiKey could not reach the guard, so the row has to write a flows object. Dropping the Kind guard now reddens exactly that row of TestLowerSecuritySchemes_AnOAuth2SchemeWithNoFlowIsReported. The PR body's mutation list is corrected too. b87282c
| } | ||
|
|
||
| // TestAuth_OAuthNoFlowsUnknownTypeAndGhostRef pins three entries beside a | ||
| // document that declares nothing wrong with them: an oauth2 scheme with no |
There was a problem hiding this comment.
nit: "beside a document that declares nothing wrong with them" isn't accurate, since two of the three entries are defective (the flowless oauth2 entry and the ghost $ref). "which is now reported" is change history rather than what the test pins. Further down, "the loader's other spelling" is confusing, because the entry here, {type: oauth2}, is the absent spelling itself. Something like "pins three entries a document can carry: an oauth2 scheme with no flow, reported at its own entry and interned anyway; ..." avoids both.
There was a problem hiding this comment.
Reworded along the lines you suggested: "pins three entries a document can carry: an oauth2 scheme with no flows object, interned with nil flows and reported only by the loader, …". The "loader's other spelling" passage is gone. That entry is the absent spelling, and the test now asserts it draws no oauth2-no-flow report. b87282c
| // because the loader cannot place it: it accepts `flows: {}` and reports an | ||
| // absent `flows` with no pointer (GitHub #646). | ||
| // | ||
| // Reported, not refused as IncompleteSecurityScheme is: the IR states exactly |
There was a problem hiding this comment.
"The IR states exactly what the document said" is the argument for interning, but ir/auth.go:16 still documents AuthKindOAuth2 as "OAuth 2.0 with one or more flows". After this PR a flowless oauth2 scheme is a deliberately interned, documented shape, so that comment is now imprecise about a shape the compiler is designed to produce (and has produced silently since #4). Nothing in docs/ or irverify states or checks a flow count, so I found no emitter-facing contract broken. But a reader of ir.AuthKindOAuth2 is told the opposite of what this record says. Amending that line ("with zero or more flows; none when the document declares none, which a compiler reports") would keep the two in step.
There was a problem hiding this comment.
Amended as suggested: AuthKindOAuth2 is now "OAuth 2.0 with zero or more flows; none when the document declares none, which a compiler reports." b87282c
| if scheme.Kind == ir.AuthKindOAuth2 && len(scheme.Flows) == 0 { | ||
| // Reported rather than refused — see oauthNoFlowDiag — and reported | ||
| // before the lines below, which is why those append into diags. | ||
| diags = append(diags, oauthNoFlowDiag(c, name, entry)) |
There was a problem hiding this comment.
The placement contradicts a rule this function states and a sibling test pins. lowerSecurityScheme's doc says entry is where the document names the scheme and places "anything said of the entry as a whole", while decl is "where the fields live". TestLowerSecuritySchemes_ARefdEntryRecordsFieldsWhereTheyAreWritten asserts "nothing is reported against a position the alias does not hold" and "two aliases of one declaration report it once", through the dedup on the whole diagnostic value in compile.Diags.
A flowless declaration is a fact about the declaration's text. This reports it at each alias's {$ref: ...} object, which holds no flow either way, and embeds the alias name in the message so the dedup cannot collapse the copies. One defect with N aliases gives N+1 warnings, and none is at the place to edit. The cited #107 concerns fabricated hoist pointers and doesn't mention security schemes.
Report at decl with a message that doesn't embed the entry name (the preserveUnreadFields messages don't either), and the copies dedup. Prototype: a target plus two aliases gives one warning at /components/securitySchemes/target, a three-deep alias chain gives one at the final declaration, and an unaliased scheme reports where it did before. TestLowerSecuritySchemes_AFlowlessRefEntryIsReportedAtEachEntry would then pin the rule as the function's doc states it. Switching the call to decl alone reddens only that test, so the PR's tests already pin the current choice.
There was a problem hiding this comment.
Agreed. It's now reported at decl, and the message names no entry ("oauth2 security scheme declares no flow: no token endpoint is stated for it"), so the copies dedup. A target with two aliases gives one warning at /components/securitySchemes/target, and a three-deep alias chain gives one at the final declaration. The test is now TestLowerSecuritySchemes_AFlowlessDeclarationIsReportedOnceWhereItIsWritten, with both aliases declared before the target. Switching the call back to entry reddens it and TestConformance. The #107 citation is dropped from the code and the PR body. b87282c
| // lowers to AuthKindOAuth2 with an empty flow list — which is why one rule | ||
| // covers them and why the report has to be sited here: the loader refuses only | ||
| // the absent spelling, and sites that finding at no pointer at all. | ||
| func TestLowerSecuritySchemes_AnOAuth2SchemeWithNoFlowIsReported(t *testing.T) { |
There was a problem hiding this comment.
Nothing the repo's sweeps reach exercises this rule, so everything outside this package's unit tests is blind to it. I deleted the feature (if false && ... on the new branch, which compiles) and ran ./compilers/openapi, ./internal/harness/..., ./ir/..., ./pass/... and ./cmd/.... All stayed green, including TestConformance, the goldens and TestHarness_InRepoCorpus. Compiling the 154 specs under testdata/, none draws openapi/oauth2-no-flow; the four that declare type: oauth2 all declare a flow.
CLAUDE.md names the way to cover a new construct: add a spec the sweep reaches, so the no-panic, irverify, round-trip, determinism and order-invariance oracles run over it, and a golden pins the diagnostic's code, pointer and severity byte for byte. The PR's test plan checks the existing fixtures and leaves testdata/ alone, which is the gap. A conformance spec (testdata/conformance/openapi/: .yaml, .golden.json and a table row) should hold flows: {}, a flows object with only unknown keys, an aliased flowless scheme with the alias declared before its target (the order that would be wrong under a canonical-site bug), and a metadata-only 3.2.0 scheme as the silent control. I ran the harness on probes of these shapes and they pass, so adding them should be safe.
There was a problem hiding this comment.
Added testdata/conformance/openapi/oauth2-no-flow.yaml with its golden and a table row (assertOAuth2NoFlow). It holds flows: {}, a flows object with only an unknown key, two aliases declared before their flowless target, and two silent controls: a 3.2.0 metadata-only scheme and a declared flow. morphic-harness passes on it. With the feature deleted (if false && …), TestConformance now reddens along with the unit tests. b87282c
|
Verdict: not yet at the bar. The core is sound:
Smaller: the IR's "one or more flows" text now misdescribes a shape this PR deliberately interns ( I prototyped 1 to 3. The whole production change is: if scheme.Kind == ir.AuthKindOAuth2 && ss.GetFlows() != nil &&
len(scheme.Flows) == 0 && scheme.OAuth2MetadataURL == "" {
diags = append(diags, oauthNoFlowDiag(c, decl))
}
// oauthNoFlowDiag message: "oauth2 security scheme declares no flow: no token endpoint is stated for it"Results against it: a target plus two aliases gives one warning at the target, a three-deep chain gives one at the final declaration, absent The PR body should also drop two claims that this changes: "one rule for every spelling", and the #107 citation for per-entry reporting. |
The oauth2-no-flow warning now fires only for what the loader cannot
see: a flows object that is present but declares no flow, on a scheme
with no oauth2MetadataUrl.
- An absent flows object is a REQUIRED-field error the loader already
reports, with a line and column. Reporting it again gave one defect
two severities and made the exit code depend on the spelling.
- A 3.2 metadata URL is RFC 8414 metadata, which states the token
endpoint, so a metadata-only scheme is valid and usable.
- The report is placed at the declaration, and its message names no
entry, so N aliases of one flowless declaration give one warning
where the fields are written instead of N+1 at {$ref} objects.
A conformance spec puts the rule in reach of the corpus sweeps, a
reflection test holds presentFlows to every flow field soa.OAuthFlows
declares, an apiKey row pins the Kind guard, and ir.AuthKindOAuth2 no
longer claims one or more flows.
|
Thanks for the thorough pass. All four gaps and the smaller notes are addressed in b87282c, each one probed before the change:
The smaller notes:
The PR body is updated: it no longer claims one rule for every spelling or cites #107, and the mutation list is corrected. On the merge commit trailers: noted, and the squash body will be set by hand at merge time. |
2 participants
Summary
{type: oauth2, flows: {}}is structurally valid — every member ofOAuthFlowsis optional — so it lowers to anAuthKindOAuth2scheme with an empty flow list and no report from anywhere. An oauth2 scheme states no token endpoint without a flow, so the entry is unusable, and a requirement naming it keeps a liveAuthIDfor a scheme that says nothing an emitter can implement (#646).Rule. An oauth2 entry whose
flowsobject is present but names no flow (flows: {}, or only keys this model does not name), and which has nooauth2MetadataUrl, gets one warning at its declaration. An absentflowsis left to the loader's required-field error.Code, severity, placement. New code
openapi/oauth2-no-flow(diag.OAuth2NoFlow), warning, at the declaration, once however many aliases reach it. The loader acceptsflows: {}, so only the compiler can see that shape. The spelling the loader does refuse,flowsabsent, it reports asvalidation-required-fieldat a line and column. Reporting that again would give one defect two diagnostics and two severities, and make the exit code depend on the spelling. A flowless declaration is a fact about the text where its fields are written, and a{$ref: …}entry holds no flows either way. So the report sits atdecl, and its message names no entry, which lets the copies every alias draws dedup to one.The scheme still interns — deliberately not the refusal #294 points at. #294 refused an entry naming no mechanism because
ir.AuthKindhas no value for "the document did not say which mechanism": interning would have asserted that the API is authenticated by nothing in particular. Here the IR states exactly what the document said —oauth2, with no flows — so nothing is misstated and nothing must be refused. Refusing would also delete declared text (description,x-*,oauth2MetadataUrl) and collapse every requirement naming the scheme, for a document defect the IR can hold. The compiler already interns the two equally unusable siblings —apiKeywith noname,openIdConnectwith no URL — so wire-level usability is not the line it draws; the mechanism token is (fillSchemeKind), and that line is unchanged.Warning, not error. The document lowers whole, and the shape is the same class as
DisjointVisibility(exactly represented, unusable),EmptyEnum,ReservedHeaderName,InvalidMethodKeyandInvalidLocationKeyword. An error would also stopharness.Checkat the first error diagnostic and keep any fixture carrying the shape out ofirverify;--fail-on warningis the caller's lever.The metadata URL exempts the entry. In 3.2.0
oauth2MetadataUrlpoints at RFC 8414 authorization-server metadata, which states the token endpoint, andflowsis REQUIRED, so{type: oauth2, flows: {}, oauth2MetadataUrl: …}is the only way to write a valid metadata-only scheme. The exemption follows the IR field, so it also applies when a 3.1 document carries the URL, since the library reads it there too.Where it lives.
compilers/openapi/internal/diag/diag.go:diag.OAuth2NoFlowwith its design record, immediately afterIncompleteSecurityScheme.compilers/openapi/internal/auth/auth.go:declaresNoFlow(the predicate) andoauthNoFlowDiagbesidemechanismRefusalDiag, and the branch after thenamedguard inlowerSecurityScheme.fillSchemeKind's oauth2 case records that an absent or empty flows object lowers to nil flows, andLowerSecuritySchemes' doc names the reporter of an entry that did intern.ir/auth.go:AuthKindOAuth2now documents zero or more flows, since a flowless scheme is a shape the compiler deliberately produces. Noir/type changes, noOptionsswitch, noirverifyrule: nothing is inferred, and holding flow fields inirverifywould re-validate the source.Test plan
TestLowerSecuritySchemes_AnOAuth2SchemeWithNoFlowIsReportedpins the rule row by row:flows: {}(the issue repro): one warning at/components/securitySchemes/s.flowsabsent: nooauth2-no-flow, only the loader's error.flows: {application: …}: the warning, plus the existingunknown-object-keyat/components/securitySchemes/s/flows/application.flows: {}with anoauth2MetadataUrlunder 3.2.0: silent, and the URL is kept.flows: {}: silent, with the stray field kept. This row pins the Kind guard.{flows: {}}with notype: onlyincomplete-security-scheme.Every interned row also pins that the requirement naming the scheme keeps a live
AuthID.TestLowerSecuritySchemes_AFlowlessDeclarationIsReportedOnceWhereItIsWritten: two$refaliases, declared before the flowless target they share, all intern, and the one declaration is reported once at/components/securitySchemes/target, with nothing at either alias.TestAuth_OAuthNoFlowsUnknownTypeAndGhostRefasserts the absent spelling draws nooauth2-no-flowreport and that the loader still reports it.TestPresentFlows_ReadsEverySourceFlowFieldsets each flow field ofsoa.OAuthFlowsalone, by reflection, and requirespresentFlowsto read it. The report reads an empty lowered list as "declares no flow", which holds only while every flow field is read. Commenting out the password flow reddens it.New conformance spec
testdata/conformance/openapi/oauth2-no-flow.yaml, with its golden and table row. It holdsflows: {}, a flows object with only an unknown key, two aliases declared before their flowless target, and two silent controls: a 3.2.0 metadata-only scheme and a declared flow.morphic-harnesspasses on it, so the corpus oracles now reach the rule.Mutations, each planted, observed and reverted:
if false && …) →TestConformance, the placement test and the table test redden;flowscheck →TestAuth_OAuthNoFlowsUnknownTypeAndGhostRefand the table test redden;TestConformanceand the table test redden;entryinstead ofdecl→TestConformanceand the placement test redden.go test ./...andmake lint(0 issues) on b87282c.make gateon b87282c (fmt, vet, lint, build, coverage at 100%, fuzz, bench-smoke) passes.Deliberately out of scope
tokenUrl,authorizationUrlorscopes): that is the loader's own finding, and not what "declares no flow" means.irverifyrule: holding flow fields there would re-validate the source, which its own doc rules out.securityScheme.flows is requiredfinding for an absentflowsis left exactly as it is, and is the only report of that spelling. Owning every spelling in the loader would need a node-based predicate, since the library error has no structured field identity. That belongs in a separate change.Closes #646.